-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rewrite DbBuffer Read/Write methods with stackalloc #88395
Conversation
Tagging subscribers to this area: @roji, @ajcvickers Issue DetailsA memory optimisation for DbBuffer class, avoid GC allocation when possible
|
src/libraries/System.Data.Odbc/src/Common/System/Data/ProviderBase/DbBuffer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Data.Odbc/src/Common/System/Data/ProviderBase/DbBuffer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Data.Odbc/src/Common/System/Data/ProviderBase/DbBuffer.cs
Show resolved
Hide resolved
- rewrite WriteGuid
src/libraries/System.Data.Odbc/src/Common/System/Data/ProviderBase/DbBuffer.cs
Outdated
Show resolved
Hide resolved
runtime/src/libraries/System.Data.Odbc/src/Common/System/Data/ProviderBase/DbBuffer.cs Line 476 in dff8568
byte* addr = (byte*)DangerousGetHandle() + (uint)offset;
*addr = value; |
src/libraries/System.Data.Odbc/src/Common/System/Data/ProviderBase/DbBuffer.cs
Show resolved
Hide resolved
src/libraries/System.Data.Odbc/src/Common/System/Data/ProviderBase/DbBuffer.cs
Outdated
Show resolved
Hide resolved
On a code style note, it would be preferable to use the |
src/libraries/System.Data.Odbc/src/Common/System/Data/ProviderBase/DbBuffer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Data.Odbc/src/Common/System/Data/ProviderBase/DbBuffer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Data.Odbc/src/Common/System/Data/ProviderBase/DbBuffer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Data.Odbc/src/Common/System/Data/ProviderBase/DbBuffer.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Data.Odbc/src/Common/System/Data/ProviderBase/DbBuffer.cs
Outdated
Show resolved
Hide resolved
|
||
Validate(offset, sizeof(Guid)); | ||
|
||
Debug.Assert(0 == offset % /* alignof(Guid) */ sizeof(int), "invalid alignment"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roji, @ajcvickers Note for code owners, this change relaxes the alignment requirement from ADP.PtrSize
. I believe this is acceptable, but it is still a change in behavior.
try | ||
{ | ||
DangerousAddRef(ref mustRelease); | ||
*((Guid*)DangerousGetHandle() + (uint)offset) = value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roji, @ajcvickers Note for code owners, please check whether this cast meets alignment requirements, or whether Unsafe.WriteUnaligned
should be used instead. Notably, the value offset
is only validated in the debug build.
Span<short> spanBuffer = stackalloc short[3]; | ||
ReadInt16Array(offset, spanBuffer.Slice(0, 3)); | ||
return new DateTime( | ||
unchecked((ushort)buffer[0]), // Year | ||
unchecked((ushort)buffer[1]), // Month | ||
unchecked((ushort)buffer[2])); // Day | ||
unchecked((ushort)spanBuffer[0]), // Year | ||
unchecked((ushort)spanBuffer[1]), // Month | ||
unchecked((ushort)spanBuffer[2])); // Day |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also be rewritten as unsafe method:
[StructLayout(LayoutKind.Sequential)]
public unsafe readonly struct DateData
{
public readonly ushort Year;
public readonly ushort Month;
public readonly ushort Day;
}
DateData data = *(DateData*)addr;
return new DateTime(data.Year, data.Month, data.Day);
@Poppyto thanks for the contribution, sorry for the delay in responding. @roji @ajcvickers do we want to accept this kind of change into Odbc? It does not have a readme indicating we only take necessary changes (like eg codedom has). If we aren't taking changes like this, we should create a readme; if we are, hopefuly we can review the change. |
@SamMonoRT, what would you like to see happen with this PR? Thanks. |
Following up |
We are not taking changes to System.Data.Odbc. We will add a ReadMe for the same. cc @roji |
A memory optimisation for DbBuffer class, avoid GC allocation when possible